Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

plan: fix a problem caused by union's schema #7680

Merged
merged 11 commits into from
Sep 20, 2018
Merged

plan: fix a problem caused by union's schema #7680

merged 11 commits into from
Sep 20, 2018

Conversation

winoros
Copy link
Member

@winoros winoros commented Sep 12, 2018

What problem does this PR solve?

Before this commit. Union use the schema of its Children[0].
The Columns information is correct.
But the unique key information is not, obviously.

What is changed and how it works?

Give it a schema held by itself.

Check List

Tests

  • Unit test

Code changes

  • Has exported variable/fields change
  • Has persistent data change

This is found by #7676

@winoros
Copy link
Member Author

winoros commented Sep 12, 2018

We often treat the Schema as a list of Columns But actually it is not.
We need a ColumnList or ColumnIDList.

@winoros winoros mentioned this pull request Sep 12, 2018
Merged
@eurekaka eurekaka added the sig/planner SIG: Planner label Sep 13, 2018
@shenli
Copy link
Member

shenli commented Sep 13, 2018

I don't understand this. Could you please explain a few more?

We often treat the Schema as a list of Columns But actually it is not.
We need a ColumnList or ColumnIDList.

@shenli shenli added the type/bugfix This PR fixes a bug. label Sep 13, 2018
shenli
shenli reviewed Sep 13, 2018
View reviewed changes
plan/logical_plan_builder.go Outdated
@@ -660,15 +660,10 @@ func (b *planBuilder) buildProjection4Union(u *LogicalUnionAll) {
if _, isProj := child.(*LogicalProjection); needProjection || !isProj {
b.optFlag |= flagEliminateProjection
proj := LogicalProjection{Exprs: exprs}.init(b.ctx)
if childID == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generated new columns above here. So don't need add this check.

eurekaka
eurekaka reviewed Sep 13, 2018
View reviewed changes
plan/logical_plan_builder.go
childTp := u.children[j].Schema().Columns[i].RetType
resultTp = unionJoinFieldType(resultTp, childTp)
}
unionCols = append(unionCols, &expression.Column{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are using ColName of children[0]'s Column here, should we keep other fields except UniqueID same with children[0]'s Column as well? such as OrigTblName, OrigColName

plan/logical_plan_builder.go Outdated Show resolved Hide resolved
plan/rule_column_pruning.go
@@ -132,6 +132,7 @@ func (p *LogicalUnionAll) PruneColumns(parentUsedCols []*expression.Column) {
for _, child := range p.Children() {
child.PruneColumns(parentUsedCols)
}
p.schema.Columns = p.children[0].Schema().Columns
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? parent columns may have different UniqueID and RetType now?

Copy link
Member Author

@winoros winoros Sep 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes we need some changing. We need to make sure it's the same with its children[0]'s.
Yes, union's col's id is the same with child's

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here https://github.com/pingcap/tidb/pull/7680/files#diff-638d8be162d442ef3a5c423ff0c735d4R642 the UniqueID of columns in UnionAll is different from child[0]

union's col's id is the same with child's

What does id in the above sentence mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done by here

plan/logical_plan_builder.go Outdated
@@ -660,15 +660,10 @@ func (b *planBuilder) buildProjection4Union(u *LogicalUnionAll) {
if _, isProj := child.(*LogicalProjection); needProjection || !isProj {
b.optFlag |= flagEliminateProjection
proj := LogicalProjection{Exprs: exprs}.init(b.ctx)
if childID == 0 {
for _, col := range unionSchema.Columns {
col.UniqueID = b.ctx.GetSessionVars().AllocPlanColumnID()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if all children of LogicalUnionAll have same RetType for each Column? We would still generate new UniqueID for each Column? Can we just use previous UniqueID in this case?

Copy link
Member Author

@winoros winoros Sep 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do this.

@winoros
Copy link
Member Author

winoros commented Sep 13, 2018 •
edited
Loading

@shenli
Schema in logical planning is actually logical property.
It has not only a []*Columns but also []KeyInfo and TblID2Handle map[int64][]*Column.
But we use is as a alias of []Column in many places like

func doSomethingWithCols(other vars, cols []*Column) {
	tmpSchema := NewSchema(cols...)
	tmpSchema.Contains(colA) or tmpSchema.ColumnIndex(ColA) or something
	...
}

It would be better if we have type ColumnList []*Column. Then it can be

func doSomethingWithCols(other vars, cols ColumnList) {
	cols.Contains(colA) or cols.Index(ColA) or something
	...
}

@shenli
Copy link
Member

shenli commented Sep 13, 2018

@winoros Got it. It is much clear now. Thanks!

@winoros
Copy link
Member Author

winoros commented Sep 13, 2018

@eurekaka I've updated the comment. You can see whether is clear enough.

zz-jason
zz-jason reviewed Sep 14, 2018
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zz-jason
Copy link
Member

/run-all-tests

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 14, 2018
@winoros
Copy link
Member Author

winoros commented Sep 17, 2018

Always at a projection when building a union.
It can be removed later.

@shenli
Copy link
Member

shenli commented Sep 17, 2018

/run-all-tests

@winoros
Copy link
Member Author

winoros commented Sep 17, 2018

Oh plan id changed after the last commit.
I'll fix it.

@zz-jason
Copy link
Member

/run-all-tests

@zz-jason
Copy link
Member

@eurekaka @lamxTyler PTAL

eurekaka
eurekaka approved these changes Sep 20, 2018
View reviewed changes
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 20, 2018
@winoros winoros merged commit 5671382 into pingcap:master Sep 20, 2018
@winoros winoros deleted the union-schema branch September 20, 2018 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zz-jason zz-jason zz-jason left review comments

@shenli shenli shenli left review comments

@eurekaka eurekaka eurekaka approved these changes

Assignees
No one assigned
Labels
sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@winoros @shenli @zz-jason @eurekaka